Skip to content

feat(cli): expose ACP session configuration - #4051

Open
Sun-GLiang wants to merge 8 commits into
apache:mainfrom
Sun-GLiang:feat/acp-session-configuration
Open

feat(cli): expose ACP session configuration#4051
Sun-GLiang wants to merge 8 commits into
apache:mainfrom
Sun-GLiang:feat/acp-session-configuration

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Return ordered ACP select configOptions from the Runtime Host's committed Session projection for every successful session/new: permission mode, collaboration mode, orchestration mode, and thinking level only when the selected model advertises supported thinking levels.
  • Derive selectable values from the canonical core value lists. permission_mode advertises ask and bypass; a pre-existing explore Session may still report explore as its current value, but ACP cannot select it.
  • Resolve thinking choices from the selected model in the Runtime Host connection catalog. Non-reasoning models omit thinking_level; reasoning models advertise default plus exactly their declared levels.
  • Track Session ownership per ACP connection without opening Runtime Host subscriptions; session/list never grants ownership.
  • Implement ACP v1 session/set_config_option through the official SDK route, validating ownership, id, type, and value before Host I/O.
  • Apply each live change as a one-field SessionConfigurationPatch CAS update, preserving model selection and unrelated fields, with at most three attempts on revision conflicts.
  • Share Session lookup/projection and bounded CAS retry through a neutral CLI module while keeping ACP close-state checks and structured RequestError mapping at the ACP boundary.
  • Keep session/set_mode unsupported and unadvertised.

Refs #3132
Depends on #3744 (merged)

Review dependency

  • Rebuilt on current main at 1581dc1c4 after feat(cli): add ACP session creation and listing #3744 merged, so this PR contains only the PR3 delta.
  • Scope is limited to the ACP CLI layer and its tests; no Runtime Host or design-document changes are included.
  • Addressed the configuration review findings in aee2b5725: canonical/model-aware options, non-selectable explore, neutral shared CAS logic, and Host-backed subprocess coverage.

Error and acceptance boundaries

  • Runtime-state conflicts such as session_busy and operation_conflict remain internal ACP errors with the concrete Host code preserved in error.data.code; they are not classified as JSON-RPC invalidParams because unchanged parameters may succeed after the conflicting state clears.
  • The shared CAS module owns only neutral Session lookup/projection and bounded retry. ACP retains lifecycle checks and ACP-specific error mapping; the CLI/TUI session driver retains its own boundary behavior.
  • Reproducible Host-backed integration tests are the acceptance gate. A separate real-client manual smoke test remains useful but is supplementary rather than the primary merge criterion.

Verification

  • npm run build
  • Focused ACP and shared Session-update suites — 58 passed, 0 failed.
  • TMPDIR=/private/tmp npm --workspace maka-agent run test:dist — 772 tests: 769 passed, 3 skipped, 0 failed.
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • npm run check:asf-headers — submitted source files pass; the local checkout also contains unrelated untracked planning documents outside this PR.
  • git diff --check
  • Final diff review found no additional actionable issues.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex assisted with design, implementation, tests, verification, checklist alignment, review follow-up, and documentation updates.

Checklist

  • Tests cover the change and fail without it
  • Build, lint, format, typecheck, ASF headers, and affected/full CLI suites pass for the submitted change

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Sun-GLiang
Sun-GLiang force-pushed the feat/acp-session-configuration branch from 0bca0ed to dda8f4b Compare September 2, 2026 10:42
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review September 2, 2026 10:50

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The +1606 is misleading in a good way: production is about 306 lines across three files, the rest is tests. And the important structural choices are right. It uses the SDK's own session.setConfigOption rather than inventing an extension method, declines session/set_mode explicitly with a test asserting -32601, and lands on the Host's session.configuration.update with expectedRevision CAS instead of mirroring session state in the adapter. acp-child-process.test.ts is a real end-to-end path, actual subprocess, actual Host, session/new then set_config_option then read back. That is better evidence than most PRs bring.

Four things pull away from the existing seam, and they are the same fix repeated.

[P2] The thinking menu is a static list of eight, ignoring what the model declares

projectAcpSessionConfigOptions takes only the session, so THINKING_SPEC ships all eight levels regardless of model. The Host is fail-closed: session-catalog-coordinator.ts:881 rejects any level not in thinkingVariantsForConnection(...) with invalid_request, which this PR maps to invalidParams.

So the menu offers options that must fail. claude-sonnet-5 declares ['low','medium','high','xhigh','max'], so off and minimal always fail. deepseek-v4-flash declares ['low','high','max'], so four of eight fail. A non-reasoning model returns [], where the UI is supposed to hide the switch entirely and here seven of eight fail.

model-thinking.ts:335 says it is "the single place that decides which models expose the knob", and the same package already does this correctly: pi-tui-runner.ts:1270 reads currentModelChoice()?.thinkingLevels ?? [] and refuses rather than offering a menu when it is empty. The per-model list is already in the connection catalog projection, so no protocol change is needed. If deriving it is out of scope for this slice, not shipping thinking_level yet beats shipping a menu where most entries error.

[P2] explore is offered as a user-selectable permission mode

PERMISSION_SPEC.options includes ['explore', 'Explore']. settings.ts:486 is unusually direct about this:

explore is excluded, it's reserved for Deep Research sessions and Bot-incoming guards and is never a mode the user picks, in the composer dropdown or here. Derived from the canonical PERMISSION_MODES (not a hand-copied literal) so adding a future mode updates every consumer in one place.

That comment names both of the things this PR does. CHAT_DEFAULT_PERMISSION_MODES is the list every existing picker uses via PERMISSION_MODE_ORDER. Note currentValue still needs to be able to say explore, since a Deep Research session projects as that.

[P2] The four enums are hand-copied, so the adapter becomes a stricter second gate

PERMISSION_SPEC, THINKING_SPEC, COLLABORATION_SPEC and ORCHESTRATION_SPEC each restate values that live in @maka/core (permission.ts:26, orchestration.ts:20, collaboration.ts:20, model-thinking.ts:49). Add a value to any of them later and ACP will not only miss the new option, validateAcpSessionConfigOptionRequest will reject it as unsupported even though the Host accepts it. Deriving the options and keeping only a Record<Value, DisplayName> makes a missed value a compile error, and is the same edit as the two findings above.

[P2] Third copy of the CAS retry loop in one package

#setConfigOption's retry loop, #getConfigurableSession, requireConfigurableSession and ACP_SESSION_CONFIGURATION_MAX_ATTEMPTS = 3 are line-for-line siblings of updateRuntimeHostSession, getRuntimeHostSession, requireSession and MAX_CATALOG_ATTEMPTS = 3 in runtime-host-session-driver.ts:1767, same package. Exporting those and calling them leaves #setConfigOption with only the ACP-specific parts, and is a net deletion.

Smaller notes: session_busy and operation_conflict map to internalError where invalidParams fits, both are client-correctable. And #ownedSessionIds looks redundant against the Host authority, but it actually blocks something real, session-manager.ts:1117 silently strips the Deep Research label when a Deep Research session's permission mode moves off explore. One comment saying what it guards would stop it being deleted as redundant state later.

Next step

The four P2s converge into one edit and production comes out smaller. Two test cases that assert eight thinking levels and three permission modes will need to change with them.

Then manual acceptance with a real ACP client against maka --acp, since this is user-visible and acp-child-process.test.ts only exercises collaboration_mode: after session/new, check that each dropdown's options match what the session's model and the product actually allow, and set each one once. That is exactly where the first two findings live.

One scheduling note, not a defect: session/prompt is not registered yet, so a client can create and configure a session but cannot drive a turn. Fine as a slice of #3132, worth being deliberate about in merge order.

Evidence boundary: read the full production diff, all four acp/ files at head, the Host coordinator's configuration path, the core enum modules, and the SDK 1.4.0 types. I did not read the 881-line registry test body beyond its case titles, and ran no build, test or typecheck.

AI-assisted review: drafted with Maka.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I pushed aee2b5725 with the concrete fixes:

  • ACP configuration values now derive from the canonical core value lists.
  • Thinking options are projected from the selected model in the Runtime Host connection catalog and are omitted for models with no thinking levels.
  • explore is no longer advertised as a selectable permission mode (an existing session may still report it as its current value).
  • The duplicated Session CAS loop is now shared through a small neutral runtime-host-session-update.ts module and reused by both the Runtime Host session driver and ACP.
  • Added real child-process + Runtime Host coverage for model-specific thinking options, non-thinking models, all four setting flows, and the non-selectability of explore.

I kept three boundaries intentionally:

  1. session_busy / operation_conflict are not mapped to invalidParams. These are runtime-state/concurrency failures, not malformed method parameters; the same request and parameters may succeed when retried later. They remain on the internal-error path while preserving the concrete Host code in error.data.code. This avoids assigning JSON-RPC -32602 semantics to a transient state conflict.

  2. The shared CAS code is neutral rather than exported directly from runtime-host-session-driver.ts. The driver helper previously carried CLI/TUI-style plain-Error behavior. ACP still needs its own close-state checks and structured RequestError mapping, so the shared module owns only Session lookup/projection and bounded CAS retry, while each caller retains its boundary policy.

  3. Reproducible Host-backed integration tests are the acceptance gate. The new subprocess test uses the ACP SDK client against the real ACP process and Runtime Host, exercises all four configuration updates, verifies model-specific thinking values, and verifies that explore is not selectable. A separate manual client smoke test can still be useful, but I do not think an unreproducible manual step should be the primary merge criterion.

Verification after the change: full CLI suite 772 tests / 769 passed / 3 skipped / 0 failed, plus repository build, typecheck, lint, and format checks all passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants